I've installed intervention/image v3 and made the roughest adjustments to get an overview of the further work required. Looking at the tests there seems to be some changes required with the encoding, because this modifier has to be run at the last step.

Hey @Art4 thanks for using my PR and making all those changes.

The only thing I'm concerned is about sizing. Glide v2 relied a lot of Intervention aspectRatio method to keep images resizing properly https://image.intervention.io/v2/api/resize

Since this useful helper was removed from the official intervention v3, I wonder what we could do for this refactor to make sure it will behave as the previous version. Specially w/ fit parameter https://glide.thephpleague.com/1.0/api/size/#fit-fit

Since this useful helper was removed from the official intervention v3, I wonder what we could do for this refactor to make sure it will behave as the previous version. Specially w/ fit parameter https://glide.thephpleague.com/1.0/api/size/#fit-fit

We could also consider changing Glide's API/options as per changes to Image, if trying to retain existing options is too problematic.

For e.g. I quite like the new "cover", "contain" naming in Image as they corelate to similar behavior in CSS.

I'm nearly done with fixing all tests, but are struggling with Mockery on this last test.
See https://github.com/Art4/glide/actions/runs/7656671526/job/20865541487

Code:

Tests:

Error: 1) League\Glide\Manipulators\BorderTest::testRunOverlay Mockery\Exception\InvalidCountException: Method width(<Any Arguments>) from Mockery_1_IteratorAggregate_Intervention_Image_Interfaces_ImageInterface should be called exactly 1 times but called 0 times.

Could someone help me why the provided closure is not called?

@Art4 Since the drawRectangle() which uses the closure is mocked the closure would never actually be used/run.

@Art4 I fixed the test and pushed the changes to your branch.

@Art4 I just have one objection with the current strategy to encode the image.

It seems the Encode Manipulator was commented out from ServerFactory::getManipulators() and moved to Api::run. I can see the reason why it was made.

But I still think that the Encode Manipulator is still vital for this and my suggestion is to keep that, unless @ADmad has a different opinion.

I managed to fix the Encoded Manipulator on my branch and I was able to test it on production properly after the changes I did here Art4#2. Would you mind to check it again?

@konnng-dev Yes we should keep the encoder manipulator since you have managed to fix it.

It seems the Encode Manipulator was commented out from ServerFactory::getManipulators() and moved to Api::run. I can see the reason why it was made.

Good catch. I've uncommented this, so Encode Manipulator is used again.

But I still think that the Encode Manipulator is still vital for this and my suggestion is to keep that, unless @ADmad has a different opinion.

I managed to fix the Encoded Manipulator on my branch and I was able to test it on production properly after the changes I did here Art4#2. Would you mind to check it again?

I would recommend to create a separate PR in this main repo about this issue, instead in my private repo, to keep the version history and discussions in this repo.

I still have to do a more thorough review of the PR but I am going to merge it so that @konnng-dev can submit his changes on top.

Thanks guys for all your work. Please start trying out the 3.x branch as due to the nature of this lib passing tests are no guarantee that the images generated are what we expect 😄.